Fixes #38024 - Add ordering by virtual columns #10381
Conversation
|
It seems to work as expected in ui and api, the code makes sense but I think we need a Ruby person to verify it, @adamruzicka ? |
| if virt_column.blank? || model_of_controller.columns_hash[virt_column] || !base_scope.respond_to?(select_method) | ||
| base_scope.search_for(params[:search], :order => params[:order]).paginate(:page => params[:page], :per_page => params[:per_page]) | ||
| else | ||
| base_scope.send(select_method).search_for(params[:search]).order(params[:order]).paginate(:page => params[:page], :per_page => params[:per_page]) |
There was a problem hiding this comment.
UI does not accept per_page being set to all?
There was a problem hiding this comment.
Also, thinking out loud, from the name I wouldn't really expect it to deal with pagination and so on, but that might be just me
There was a problem hiding this comment.
UI does not accept per_page being set to all?
Nope, otherwise we'd have a button suggesting that.
Also, thinking out loud, from the name I wouldn't really expect it to deal with pagination and so on, but that might be just me
Naming is hard, any suggestion is more than welcome 🍪
There was a problem hiding this comment.
Does this break per_page=all for api?
There was a problem hiding this comment.
Does this break per_page=all for api?
wrap_for_virt_column_select method is only for UI controllers, thus no.
API controllers most (if not all) of the time use resource_scope_for_index method, which is overridden in this PR:
https://github.com/theforeman/foreman/pull/10381/files#diff-f97adc72a387ed049f1b789f93f9d5c592f481e9e37c6fb75eabfcc2f36d467eR172
This override should not affect per_page=all, since it's either uses super to use old behavior or explicitly deals with it in https://github.com/theforeman/foreman/pull/10381/files#diff-f97adc72a387ed049f1b789f93f9d5c592f481e9e37c6fb75eabfcc2f36d467eR181.
ofedoren
force-pushed
the
feat-38024-allow-virt-ordering
branch
from
56d9b7a to
373036f
Compare
ofedoren
force-pushed
the
feat-38024-allow-virt-ordering
branch
from
373036f to
0039511
Compare
|
I've updated the logic a bit: since most of the UI controllers use WDYT? Should we expose the wrapper or rather force usage of already defined loaders if ordering by virt columns desired? |
ofedoren
force-pushed
the
feat-38024-allow-virt-ordering
branch
from
0039511 to
4fea540
Compare
This is an alternative for #10368, tries to make ordering by columns, which are not part of actual table, possible.
This PR enables it for roles model, making possible to click on
lockedcolumn in roles index page and see the ordering, as well as doing so via API.Added an entry for developer_docs saying how to use it within plugins.
Thanks, @adamruzicka, for the idea!
@MariaAga, what do you think?
P.S. The only issue I see is that we'd need to update https://github.com/theforeman/foreman-tasks/blob/master/app/controllers/foreman_tasks/api/tasks_controller.rb#L195 to be compatible. Could be done as part of removal deprecated stuff.